Upgrade dependencies and fix state-tracking bug#3
Conversation
indexzero
left a comment
indexzero
left a comment
There was a problem hiding this comment.
Would like to see the getters removed before merging. Overall looks like a solid refactor. Definitely a lot of complexity in all the lifecycle edge-cases. Glad to see all the unit tests appear to cover them 👍
|
|
||
| get validates() { | ||
| // Prefer props over state. | ||
| const { validates = this.state.validates } = this.props; |
There was a problem hiding this comment.
This is a great ES6-based React pattern I would not have thought of. 💯
| get ctxHandler() { | ||
| const { onValidChange = noop } = this.context; | ||
| return onValidChange; | ||
| } |
There was a problem hiding this comment.
You only use these getters once in onValidChange. Remove them and simply define them in that function scope.
| const { onValidChange: ctxHandler } = this.context; | ||
| onValidChange(isValid, wasValid, oldName) { | ||
| const { propsHandler, ctxHandler } = this; | ||
| const { name } = this.props; |
There was a problem hiding this comment.
As per above comment this would be:
const { name, propsHandler = noop } = this.props;
const { ctxHandler = noop } = this.context;| const { validates: wasValid } = this.props; | ||
| const { validates: isValid } = nextProps; | ||
| componentDidUpdate(prevProps) { | ||
| const { validates: wasValid, name: oldName } = prevProps; |
There was a problem hiding this comment.
Nitpick: prevName since it came from prevProps
| this.onValidChange(isValid, wasValid); | ||
| // Prefer props over state. | ||
| const { validates: wasValid = prevState.validates, name: oldName } = prevProps; | ||
| this.onValidChange(isValid, wasValid, oldName); |
There was a problem hiding this comment.
Nitpick: prevName since it came from prevProps
|
|
||
| const { onValidChange: propsHandler, name } = this.props; | ||
| const { onValidChange: ctxHandler } = this.context; | ||
| onValidChange(isValid, wasValid, oldName) { |
There was a problem hiding this comment.
Nitpick: rename oldName to prevName for consistency with other rename nitpicks. Nitpicks on nitpicks?
| "prepublish": "npm run build", | ||
| "build": "babel *.js -d lib" | ||
| "build": "babel *.js -d lib", | ||
| "lint": "eslint --fix *.js test", |
There was a problem hiding this comment.
Why are you not using the eslint-godaddy-react binary that is installed by the eslint-config-godaddy-react package here? This way you don't need to have a the .eslint file in the root of the repository.
There was a problem hiding this comment.
Personally, I find it nice to specify the .eslint config at the project root just to make it clear what config this project is using for tools that run eslint separately (for example, when Neovim runs it using neomake). Of course that's a bit of a selfish reason so I'm happy to remove it.
There was a problem hiding this comment.
The other argument @3rd-Eden is that the eslint-godaddy-react binaries have 💩 💩 💩 support for Windows due to how which works cross platform. Keeping it this way make contribution more accessible to Windows folks.
| "eslint-plugin-react": "^6.10.3", | ||
| "mocha": "^3.4.2", | ||
| "nyc": "^11.0.3", | ||
| "prop-types": "15.x.x", |
There was a problem hiding this comment.
could we semver range this in exactly the same way the rest of the dependencies?
| }; | ||
| } | ||
|
|
||
| get validates() { |
|
|
||
| const undef = void 0; | ||
|
|
||
| export default class Validate extends Validates { |
indexzero
dismissed
3rd-Eden’s stale review
Comments from @3rd-Eden have been addressed
* Add test to demonstrate issue * Upgrade dependencies and re-do tests to accomodate new lifecycle handler logic * More test coverage for mount/unmount * Update README docs for testing * Remove extraneous getters, `oldName` -> `prevName` * More naming changes * Use consistent semver style * Improved jsdoc
3 participants
Previously, the context and props handlers would not be called to indicate that a component's name has changed or it got unmounted. This PR fixes this issue, and adds the appropriate tests.
It also upgrades some outdated dependencies (
istanbul->nyc,godaddy-style->eslint-config-godaddy-react,react-addons-test-utils->react-test-renderer, other various version bumps) and removes the requirement forjsdomfor testing.If this gets merged, it would probably be a major version bump - it introduces extra calls to lifecycle/change handlers, as well as a new validity state. The
undefinedstate existed implicitly before (when aValidatesdid not have avalidatesprop), but it was undocumented. It now has a better-defined meaning documented in the README.